Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Enhance version_path_separator behaviour by adding a newline option #1510

Closed
wants to merge 5 commits into from
Closed

Enhance version_path_separator behaviour by adding a newline option #1510

wants to merge 5 commits into from

Conversation

pristupa
Copy link
Contributor

@pristupa pristupa commented Jul 25, 2024 •
edited
Loading

Description

version_path_separator now consists a new option "newline" which allows you to specify multiple version locations across multiple lines like this:

version_locations =
  /foo/versions
  /bar/versions
  /baz/versions

version_path_separator = newline

Checklist

This pull request is:

  • A documentation / typographical error fix
    • Good to go, no issue or tests are needed
  • A short code fix
    • please include the issue number, and create an issue if none exists, which
      must include a complete example of the issue. one line code fixes without an
      issue and demonstration will not be accepted.
    • Please include: Fixes: #<issue number> in the commit message
    • please include tests. one line code fixes without tests will not be accepted.
  • A new feature implementation
    • please include the issue number, and create an issue if none exists, which must
      include a complete example of how the feature would look.
    • Please include: Fixes: #<issue number> in the commit message
    • please include tests.

Have a nice day!

pristupa
pristupa commented Jul 25, 2024
View reviewed changes
alembic/testing/assertions.py Outdated
@@ -98,8 +101,8 @@ def expect_raises(except_cls, check_context=True):
return _expect_raises(except_cls, check_context=check_context)


def expect_raises_message(except_cls, msg, check_context=True):
return _expect_raises(except_cls, msg=msg, check_context=check_context)
def expect_raises_message(except_cls, msg, check_context=True, text_exact=False):
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function was incorrectly used for text exact checking for ValueError messages. The raw message was assumed to be a regexp but it was a simple string. So it was passed by coincidence.

@CaselIT
Copy link
Member

CaselIT commented Jul 26, 2024

Thanks, looks ok! Will run the pipeline

@CaselIT
Copy link
Member

CaselIT commented Jul 26, 2024

That error seems to be caused by the mypy update. Will fix elsewhere

@pristupa pristupa changed the title Enhance version_path_separator behaviour by adding a multiline option Enhance version_path_separator behaviour by adding a newline option Jul 26, 2024
@CaselIT CaselIT requested a review from sqla-tester July 26, 2024 19:36
sqla-tester
sqla-tester reviewed Jul 26, 2024
View reviewed changes
Copy link
Collaborator

@sqla-tester sqla-tester left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, this is sqla-tester setting up my work on behalf of CaselIT to try to get revision 00a1a04 of this pull request into gerrit so we can run tests and reviews and stuff

@sqla-tester
Copy link
Collaborator

New Gerrit review created for change 00a1a04: https://gerrit.sqlalchemy.org/c/sqlalchemy/alembic/+/5417

@CaselIT CaselIT requested a review from sqla-tester July 27, 2024 11:30
sqla-tester
sqla-tester reviewed Jul 27, 2024
View reviewed changes
Copy link
Collaborator

@sqla-tester sqla-tester left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, this is sqla-tester setting up my work on behalf of CaselIT to try to get revision a4d2a1e of this pull request into gerrit so we can run tests and reviews and stuff

@sqla-tester
Copy link
Collaborator

Patchset a4d2a1e added to existing Gerrit review https://gerrit.sqlalchemy.org/c/sqlalchemy/alembic/+/5417

@pristupa
Copy link
Contributor Author

@CaselIT should I fix those issues from the Gerrit review?

@CaselIT
Copy link
Member

CaselIT commented Jul 27, 2024

if you run black it should fix them for you

@pristupa
Copy link
Contributor Author

I've just run it with "tox e -- black ."
Hopefully it's what was assumed. Not sure it's nice formatted now though.

@CaselIT CaselIT requested a review from sqla-tester July 27, 2024 21:47
sqla-tester
sqla-tester reviewed Jul 27, 2024
View reviewed changes
Copy link
Collaborator

@sqla-tester sqla-tester left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, this is sqla-tester setting up my work on behalf of CaselIT to try to get revision b1465ec of this pull request into gerrit so we can run tests and reviews and stuff

@sqla-tester
Copy link
Collaborator

Patchset b1465ec added to existing Gerrit review https://gerrit.sqlalchemy.org/c/sqlalchemy/alembic/+/5417

sqla-tester
sqla-tester reviewed Jul 29, 2024
View reviewed changes
Copy link
Collaborator

@sqla-tester sqla-tester left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, this is sqla-tester and I see you've pinged me for review. However, user pristupa is not authorized to initiate CI jobs. Please wait for a project member to do this!

@CaselIT
Copy link
Member

CaselIT commented Jul 29, 2024

Kust waiting for mike to take a look. If you have time you could add a changelog. See example here https://github.com/sqlalchemy/alembic/tree/34dbe6afa27db5288629e1ec6fe5fbcd675a3b2f/docs/build/unreleased

@pristupa
Copy link
Contributor Author

oh thank you, didn't notice that one
added an individual changelog

@CaselIT CaselIT requested a review from sqla-tester July 30, 2024 05:45
sqla-tester
sqla-tester reviewed Jul 30, 2024
View reviewed changes
Copy link
Collaborator

@sqla-tester sqla-tester left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, this is sqla-tester setting up my work on behalf of CaselIT to try to get revision 6155da7 of this pull request into gerrit so we can run tests and reviews and stuff

@sqla-tester
Copy link
Collaborator

Patchset 6155da7 added to existing Gerrit review https://gerrit.sqlalchemy.org/c/sqlalchemy/alembic/+/5417

@sqla-tester
Copy link
Collaborator

Michael Bayer (zzzeek) wrote:

ok we need a changelog note here, do we have any tests for these version path seps?

View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/alembic/+/5417

@sqla-tester
Copy link
Collaborator

Federico Caselli (CaselIT) wrote:

it has both. maybe you looked an old version?

View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/alembic/+/5417

@sqla-tester
Copy link
Collaborator

Michael Bayer (zzzeek) wrote:

gerrit must have bounced me into an older version

View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/alembic/+/5417

@sqla-tester
Copy link
Collaborator

Gerrit review https://gerrit.sqlalchemy.org/c/sqlalchemy/alembic/+/5417 has been merged. Congratulations! :)

@CaselIT CaselIT mentioned this pull request Aug 5, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sqla-tester sqla-tester sqla-tester left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@pristupa @CaselIT @sqla-tester